Refactors for publishing on Source Academy#7
Refactors for publishing on Source Academy#7veehz wants to merge 11 commits intosource-academy:mainfrom
Conversation
There was a problem hiding this comment.
Code Review
This pull request renames the package to @sourceacademy/torch, updates project documentation, and introduces a Python bridge (pyodide_bridge.py) to allow the library to be used within Pyodide. Feedback focuses on several critical issues in the Python bridge implementation: the replacement of the example bridge file with a path string will cause syntax errors, the Tensor slicing logic is inefficient and breaks PyTorch's view semantics, and the getattr wrapper fails to pass keyword arguments to the underlying JavaScript functions. Additionally, improvements are needed for the result wrapping logic to handle non-tensor objects and to ensure _NNModule calls use the forward method correctly.
I am having trouble creating individual review comments. Click here to see my feedback.
examples/pyodide/bridge.py (1)
The file examples/pyodide/bridge.py has been replaced with a single line containing a relative path string ../../pyodide_bridge.py. However, examples/pyodide/main.js (line 18) reads this file's content and passes it directly to pyodide.runPython(). This will result in a SyntaxError in Python because a path string is not valid Python code.
You should either update main.js to read from the root pyodide_bridge.py or ensure that bridge.py contains valid Python code that imports or executes the logic from the root file.
pyodide_bridge.py (480-482)
_NNModule.__call__ attempts to invoke self._module.call(*js_args). In most JavaScript reimplementations of PyTorch, module instances are objects that expose a forward method rather than a call method. Unless the underlying JS library specifically adds a call method to all modules, this will fail. It is more standard and robust to have __call__ invoke self.forward(*args), which is already correctly implemented to call the JS forward method.
def __call__(self, *args):
return self.forward(*args)
pyodide_bridge.py (304-307)
The implementation of slicing for Tensor objects creates a new tensor by iterating over the range and converting each element to a Python list and then back to a JS tensor. This is highly inefficient for large tensors and, crucially, it returns a copy instead of a view. In PyTorch, slicing a tensor should return a view that shares the same underlying storage.
Since the JS Tensor.index method (line 351 in src/tensor.ts) already supports creating views for single indices, you should consider extending the JS implementation to support slicing or implementing a more efficient view-based slicing mechanism in the bridge to maintain PyTorch semantics.
pyodide_bridge.py (332-334)
The method wrapper returned by __getattr__ accepts **kwargs but does not pass them to the underlying JavaScript function. Many PyTorch operations rely on keyword arguments for important options (e.g., dim, keepdim, requires_grad). Ignoring these arguments will lead to incorrect behavior when they are used from Python.
Consider transforming kwargs into a JavaScript object (using to_js(kwargs)) and passing it as an argument if the JS API supports options objects.
pyodide_bridge.py (14-24)
_wrap_result automatically wraps any JsProxy returned from a JavaScript call into a Python Tensor. This is problematic if the JavaScript function returns something other than a tensor, such as a list of shapes, a boolean, or a plain configuration object. In such cases, the returned value will be incorrectly converted into a Tensor object.
You should add a check to verify that the JsProxy actually represents a tensor (e.g., by checking for the existence of a shape or id property) before wrapping it.
There was a problem hiding this comment.
Pull request overview
This PR refactors the project for publishing under the scoped package name @sourceacademy/torch, updates consumer references (tests/examples/HTML importmaps), and introduces a standalone Pyodide bridge file intended for distribution.
Changes:
- Renames the package from
torchto@sourceacademy/torchand updates imports/importmaps accordingly. - Adjusts the npm published file set to include
pyodide_bridge.py(and removesexamplesfrom the publishedfileslist). - Splits publishing into a dedicated GitHub Actions workflow and updates docs/CONTRIBUTING content.
Reviewed changes
Copilot reviewed 31 out of 34 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
package.json |
Renames the package scope and updates published files list to include pyodide_bridge.py. |
yarn.lock |
Updates workspace locator/name to @sourceacademy/torch. |
pyodide_bridge.py |
Adds a distributable Pyodide bridge implementing a PyTorch-like Python API over the JS library. |
.github/workflows/publish.yml |
Adds a dedicated publish workflow for package previews. |
.github/workflows/build-test.yml |
Removes publishing from CI and adjusts docs artifact preparation. |
test/*.test.{js,ts} |
Updates test imports to use @sourceacademy/torch. |
test/index.html, test/umd.html |
Updates import maps to use the new scoped package name. |
examples/pyodide/* |
Updates example dependency/imports to use @sourceacademy/torch. |
examples/basic_backpropagation.js |
Updates the Node ESM build filename extension used by the example. |
README.md |
Expands project overview and contribution guidance. |
CONTRIBUTING.md |
Adds a commands section and corrects/updates structure notes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Ensure build/test, pyodide tests, and generated-test verification run on pull requests and feature branch pushes while keeping publish and docs deployment scoped to main. Also fix CONTRIBUTING.md source links for functions/nn/optim/creation.
This PR includes multiple refactors to prepare for publishing under Source Academy:
torchto@sourceacademy/torchAdds new feature: